-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@remix-run/router
: Add support for navigation blocking
#9709
Conversation
🦋 Changeset detectedLatest commit: 7749172 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/.env | ||
/NOTES.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed somewhere to keep notes because this was getting confusing 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
return ( | ||
<> | ||
<p style={{ color: "red" }}> | ||
Blocked the last navigation to {blocker.location.pathname} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to a separate component so this wouldn't blow up on blocker.location.pathname
when in an unblocked state. everything got evaluated in the above key lookup approach.
packages/router/history.ts
Outdated
if (listener) { | ||
index = nextIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always be updating this even if no one is listening - never showed up as an issue since we always have a listener
packages/router/router.ts
Outdated
// Short circuit if navigation is blocked | ||
if ( | ||
Array.from(state.blockers).some(([_, blocker]) => { | ||
return blocker.state === "blocked"; | ||
}) | ||
) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think his is leftover form very early. Our entry points are popstate
and router.navigate
so we can just check for blocking there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// case we should log a warning as it will result in bugs. | ||
if (index == null) { | ||
index = 0; | ||
globalHistory.replaceState({ ...globalHistory.state, idx: index }, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This replaceState()
line is causing a new regression for my project when testing with Jest and JSDom. I observed the following series of circumstances:
-
The test runs in React strict mode, causing the React Router to mount twice.
-
When the React Router is loaded the 1st time,
window.location.href
ishttp://www.example.com/path/to/route
-
When the React Router is loaded the 1st time,
index
isnull
. -
The
replaceState
changeswindow.location.href
tohttp://localhost/
. Notice both the origin and the path has changed. -
When the React Router is loaded the 2nd time (strict mode), the URL is now wrong, causing the wrong route to load and my test to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose the following which fixes my problem: #10001
This PR adds support for low-level blocking of navigations within the app's location origin via
unstable_useBlocker
. We are confident in this approach but wanted to initially release it under a flag to give users an opportunity to test and provide feedback.In addition, I added an options parameter to
useBeforeUnload
so that users can have more control over how that event listener behaves when dealing with cross-origin navigations.Closes #8139.